fix: Start migrating encodePassable and rankOrder from agoric-sdk to endo #1260
+1,580
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erights
force-pushed
the
markm-binary-marshal
branch
from
de01d52
to
48ed374
Compare
erights
force-pushed
the
markm-split-marshal-vs-encode
branch
from
3467d9b
to
7c298e2
Compare
erights
force-pushed
the
markm-split-marshal-vs-encode
branch
4 times, most recently
from
12b36ca
to
b68fd00
Compare
erights
force-pushed
the
markm-binary-marshal
branch
from
48ed374
to
3a0b618
Compare
erights
force-pushed
the
markm-split-marshal-vs-encode
branch
from
ab51707
to
1447b28
Compare
kriskowal
reviewed
Aug 25, 2022
There was a problem hiding this comment.
I’m excited to see this move up and generally excited about separating the JSON/binary representation layers. I also see a lot of similarity to Syrup in this binary encoding. Similar but not the same, of course.
I can give this a closer read when it gets higher in priority.
erights
force-pushed
the
markm-binary-marshal
branch
2 times, most recently
from
429f29b
to
9a37743
Compare
warner
reviewed
Sep 1, 2022
erights
force-pushed
the
markm-binary-marshal
branch
from
97c2486
to
172a225
Compare
erights
changed the title
erights
force-pushed
the
markm-dean-smaller-json
branch
2 times, most recently
from
f0e5e87
to
ae8d7e6
Compare
erights
force-pushed
the
markm-binary-marshal
branch
2 times, most recently
from
98bb329
to
e2d0a4e
Compare
erights
force-pushed
the
markm-binary-marshal
branch
2 times, most recently
from
f08817f
to
d0403e4
Compare
erights
changed the base branch from
master
to
1303-markm-unserialize-without-assign
erights
force-pushed
the
1303-markm-unserialize-without-assign
branch
from
d63cd69
to
84a183a
Compare
Base automatically changed from
1303-markm-unserialize-without-assign
to
master
October 1, 2022 03:36
erights
force-pushed
the
markm-binary-marshal
branch
3 times, most recently
from
91bd78f
to
013fc1d
Compare
erights
force-pushed
the
markm-binary-marshal
branch
2 times, most recently
from
607e292
to
499971c
Compare
gibson042
reviewed
Oct 21, 2022
gibson042
approved these changes
Oct 24, 2022
There was a problem hiding this comment.
I feel most strongly about PassStyleRankAndCover being out-of-place in rankOrder.js (at least while it hard-codes prefix sigils from encodePassable.js), but everything appears like it will function correctly. I support merging now or fixing first, whichever is least disruptive to the relationship between endo and agoric-sdk.
gibson042
reviewed
Oct 25, 2022
gibson042
reviewed
Oct 25, 2022
erights
force-pushed
the
markm-binary-marshal
branch
from
499971c
to
8c5ad08
Compare
erights
force-pushed
the
markm-binary-marshal
branch
2 times, most recently
from
f4a2682
to
132ff75
Compare
erights
force-pushed
the
markm-binary-marshal
branch
from
132ff75
to
8ac6c3d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For our passable data types, mostly, we have a clean abstraction layering between the "marshal" layer (here in the endo repository) and the "store" layer (currently in the agoric-sdk repository). However, two modules (rankOrder and encodePassable) currently at the store layer really belong at the marshal layer.
The
encodePassableAPI and marshal'sencodeToCapDataandencodeToSmallcapsAPIs have converged to the degree that they should. They can be understood together. Before smallcaps, I originally wanted to do this in the hope thatencodePassablewould become an alternate to capData for general use, but @warner 's criticisms below are likely fatal. The remaining purpose ofencodePassableis specialized uses for which a sort-order-preserving string encoding is useful. Nevertheless, because of the conceptual similarity and layering, it is clearer for these to live here.See also Agoric/agoric-sdk#4435 which we should make progress on as part of a larger project to explain these two abstraction levels and how they relate.
See also Agoric/agoric-sdk#6260 for keeping these two in sync until the situation simplifies.